Skip to content

fix: output_bytes metric in hash aggregation#23777

Open
ariel-miculas wants to merge 2 commits into
apache:mainfrom
ariel-miculas:fix-output-bytes-metric-in-hash-agg
Open

fix: output_bytes metric in hash aggregation#23777
ariel-miculas wants to merge 2 commits into
apache:mainfrom
ariel-miculas:fix-output-bytes-metric-in-hash-agg

Conversation

@ariel-miculas

@ariel-miculas ariel-miculas commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

Summing together the produced sliced RecordBatches in hash aggregation leads to inflated output_bytes metric

What changes are included in this PR?

Are these changes tested?

Yes

Are there any user-facing changes?

If the metrics are user-facing, then yes.

@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate labels Jul 21, 2026
@codecov-commenter

codecov-commenter commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 85.92965% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.71%. Comparing base (eef1017) to head (ab748ff).
⚠️ Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
...fusion/physical-plan/src/aggregates/hash_stream.rs 81.20% 7 Missing and 21 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #23777      +/-   ##
==========================================
- Coverage   80.71%   80.71%   -0.01%     
==========================================
  Files        1089     1089              
  Lines      368748   368928     +180     
  Branches   368748   368928     +180     
==========================================
+ Hits       297633   297771     +138     
- Misses      53372    53390      +18     
- Partials    17743    17767      +24

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Jithendra2608

Jithendra2608 commented Jul 22, 2026

Copy link
Copy Markdown

Hi @ariel-miculas! I was taking a look at the failing CI checks to see if I could help unblock this.
It looks like the typos job is failing because of a small spelling error in datafusion/physical-expr-common/src/metrics/baseline.rs on line 326 (accross instead of across).

Fixing that one word should turn the CI green. Hope this helps, and great work on the hash aggregation metrics!

@ariel-miculas

ariel-miculas commented Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

@2010YOUY01 coud you please take a look?

@2010YOUY01

2010YOUY01 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Is it possible to implement it entirely within the record_output, and avoid large changes to different operators inner logic?

Also let's add some end-to-end tests, ideally we can do it with sqllogictest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Incorrect output_bytes metric in hash aggregation

4 participants

@ariel-miculas @codecov-commenter @Jithendra2608 @2010YOUY01